Repository navigation
Conversation
…and the chat scroll, streaming text and swipe settle read it
Internal, like the mark it builds on: no theme can set reducedMotion
'always' yet, so nothing changes for anyone.
- theme/reducedMotion.markers.stylex.ts defines reducedMotionScope. Theme
puts it on the marked wrapper, and the root theme writes its class on
<html> beside the mark, so a Reduce Motion style keyed on
stylex.when.ancestor('[data-astryx-reduced-motion]', reducedMotionScope)
reaches portaled layers.
- motion/reducedMotion.ts adds reducesMotion(element): the person's setting
or the mark. hooks/useReducesMotion.ts is its hook form, for a hook that
holds no element: the setting, followed live, or the theme tree through
ThemeReducesMotionContext, where a nested theme never removes an
ancestor's 'always'.
- useChatStreamScroll, useStreamingText and ItemSwipeLayer read them.
- motion/reducedMotion.test.ts guards the rest with lists that only shrink:
every Reduce Motion arm needs its twin on the mark, and every script read
of the setting needs the mark. A read through a constant that holds the
query counts.
- Tests reach 'always' through a themeMotion mock
(theme/reducedMotionAlways.testing.ts).
Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
There was a problem hiding this comment.
Changes requested. The refactor changes no behavior today and fits the theme-motion spec, but the twin contract it introduces conflicts with the private-attribute rule.
- The reduced-motion guard (
packages/core/src/motion/reducedMotion.test.ts:50-51) and the examples intheme/reducedMotion.markers.stylex.tsandmotion/reducedMotion.tsrequire each component to writestylex.when.ancestor('[data-astryx-reduced-motion]', reducedMotionScope)itself, whicharchitecture:public-component-apiINV23 forbids and@astryx/no-raw-private-attributerejects in CI; move the twin selector intoprivateAttributes/reducedMotion.stylex.tsas ready-made styles and have the guard accept a spread of them.
Non-blocking: useReducesMotion reads only the theme context, so it misses a root 'always' inside useToast's detached fallback root, and a few new comments restate the hook (see the inline notes).
[Automated review]
| const TWIN = | ||
| /stylex\.when\.ancestor\(\s*'\[data-astryx-reduced-motion\]',\s*reducedMotionScope,?\s*\)/g; |
There was a problem hiding this comment.
Blocking. This regex accepts a twin only when the component writes stylex.when.ancestor('[data-astryx-reduced-motion]', reducedMotionScope) itself. architecture:public-component-api INV23 keeps a private attribute's selectors in its owner module, and @astryx/no-raw-private-attribute reports this exact twin as an error under CI settings. Have privateAttributes/reducedMotion.stylex.ts export ready-made twin styles and let the guard accept a spread of them.
| const prefersReducedMotion = useMediaQuery( | ||
| '(prefers-reduced-motion: reduce)', | ||
| ); | ||
| const themeReducesMotion = use(ThemeReducesMotionContext); |
There was a problem hiding this comment.
Non-blocking: this reads only the theme context, so a component in useToast's detached fallback root reads false even when the root theme marks <html>, which reducesMotion(element) and the CSS twins honour. Worth closing in the change that ships 'always'.
| const leadingEntriesRef = useRef<HTMLDivElement | null>(null); | ||
| const trailingEntriesRef = useRef<HTMLDivElement | null>(null); | ||
| const isReducedMotion = useMediaQuery('(prefers-reduced-motion: reduce)'); | ||
| // The setting, live, or a theme's reducedMotion: 'always'. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates useReducesMotion's JSDoc; delete it.
| const prefersReducedMotion = useMediaQuery( | ||
| '(prefers-reduced-motion: reduce)', | ||
| ); | ||
| // The setting, live, or a theme's reducedMotion: 'always'. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates useReducesMotion's JSDoc; delete it.
| // Reduced motion skips the progressive reveal and shows the full text: the | ||
| // person's setting, followed live so a change mid-stream takes effect, or a | ||
| // theme's reducedMotion: 'always'. |
There was a problem hiding this comment.
Non-blocking (INV29): one 27-word sentence with two topics. Suggested: "Reduced motion skips the progressive reveal and shows the full text. A change mid-stream takes effect at once."
| // 'always' marks the theme's root with the attribute, its custom property | ||
| // and the reducedMotionScope marker. Script readers see it (reducesMotion, | ||
| // useReducesMotion), and each component Reduce Motion style keys a twin on | ||
| // it; motion/reducedMotion.test.ts lists the styles that still lack theirs. | ||
| // The mark only adds: a nested theme keeps an ancestor's. | ||
| // resolveMotionKnobs resolves no public theme to 'always'. |
There was a problem hiding this comment.
Non-blocking (INV29): the first sentences narrate the readers and point at a test's list. Suggested: "The mark only adds: a nested theme keeps an ancestor's. No public theme resolves to 'always' yet."
There was a problem hiding this comment.
Changes requested. The refactor changes no behavior today and fits the theme-motion spec, but the twin contract it sets up conflicts with the private-attribute rule.
- The reduced-motion guard (
packages/core/src/motion/reducedMotion.test.ts:50-51) and the examples intheme/reducedMotion.markers.stylex.tsandmotion/reducedMotion.tshave each component writestylex.when.ancestor('[data-astryx-reduced-motion]', reducedMotionScope)itself, whicharchitecture:public-component-apiINV23 forbids and@astryx/no-raw-private-attributerejects in CI; move the twin selector intoprivateAttributes/reducedMotion.stylex.tsas ready-made styles and have the guard accept a spread of them.
Non-blocking: useReducesMotion reads only the theme context, so it misses a root 'always' in a separate React root such as useToast's fallback viewport, which matters once 'always' ships. A few new comments restate the hook or narrate the code (Item/ItemSwipeLayer.tsx:208, Chat/useChatStreamScroll.ts:260, hooks/useStreamingText.ts:175-177, theme/Theme.tsx:301-306).
[Automated review]
Supersedes #8895, rebuilt on #9761 (
Depends on #9761). Part of #8677's split. This is internal: no theme can setreducedMotion: 'always'yet, so there is no behavior change.User impact
'always'ships, these changes make streaming text, the chat scroll's follow and a swipe row's settle reduce their motion for everyone under that theme, as they already do for a person whose Reduce Motion setting is on.What changes
theme/reducedMotion.markers.stylex.ts: definesreducedMotionScope.<html>beside the mark.stylex.when.ancestor('[data-astryx-reduced-motion]', reducedMotionScope)therefore reaches layers portaled to<body>.:hoverrule under a theme.motion/reducedMotion.ts: addsreducesMotion(element): the person's setting, or the mark on the element, an ancestor, or<html>.hooks/useReducesMotion.ts: its hook form, for a hook that holds no element. It reads the setting live, or the theme tree throughThemeReducesMotionContext, where a nested theme never removes an ancestor's'always'. It adds no effect.useChatStreamScroll,useStreamingTextandItemSwipeLayernow read it instead ofuseMediaQuery('(prefers-reduced-motion: reduce)').motion/reducedMotion.test.tsguards:REDUCED_MOTION_COARSE_QUERY.utils/interactionOverlay.stylex.ts,utils/pressFeedback.ts) is pending. Its'always'reader is a separate change.theme/reducedMotionAlways.testing.ts: tests reach'always'through a themeMotion mock, because no public theme can set it.[chore] Internal:.No behavior change:
resolveKnobsresolves every theme to'system', so no theme marks its root and the new branches never run in a shipped app.Earlier review findings
'always'. Markdown'sfadeInstays on the twin list until its family pull request.Evidence
theme/reducedMotionScope.test.tsx: the marker is in server markup and on<html>, and a mark-keyed style applies in the tree and in a layer portaled to<body>.'always'theme with the setting off:'system'theme, all three keep their motion.motion/reducedMotion.test.ts: both guards pass with the lists above.pnpm review:prereports no findings. check:knowledge passes.Credit
@vjeux's design and code from #8895, rebuilt on current main and #9761, with the attribute read through its owner module and the spec citations removed from comments. Co-authored-by trailer on the commit.